apps/testapp/go.mod (1)

5-5: Keep this replace local to development.

This ties the module to ../../. being present at build time. That works inside this checkout, but not when the submodule is built outside the monorepo layout. Prefer a local go.work or an uncommitted dev override instead. Please verify this module is not expected to build outside the repository layout; if apps/testapp is intentionally monorepo-only, feel free to ignore this.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/testapp/go.mod` at line 5, The committed go.mod contains a local replace
directive ("replace github.com/evstack/ev-node => ../../.") that forces a
monorepo layout; remove this replace from the tracked go.mod and instead keep it
as an uncommitted developer-only override (or move the replacement into a
go.work used only in development), or if this module is intended to be
monorepo-only, document that restriction; update the repository by deleting the
replace line from the committed go.mod and adding a dev-only go.work or
.gitignore'd override as appropriate so builds outside the repo layout are not
broken.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

pkg/p2p/client.go (1)

351-352: ⚠️ Potential issue | 🟠 Major

Backoff still turns a transient seed outage into a single retry.

At Line 351 the event is dropped if the peer is still inside its backoff window, and at Line 367 a failed host.Connect only logs. After that first failed dial there is no future trigger for that seed peer, so it can remain disconnected indefinitely.

Also applies to: 367-369

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/p2p/client.go` around lines 351 - 352, The code currently skips
processing a seed peer if time.Now().Before(st.lastAttempt.Add(backoff)) and
only logs on host.Connect failure, which means a transient outage may never be
retried; change this so that when a peer is within backoff you requeue/schedule
it for a future attempt instead of dropping the event (use a timer or push it
back onto the attempt queue and keep state in st), and when host.Connect(peer)
fails update st.lastAttempt and also schedule a follow-up retry (rather than
only logging) so the peer will be retried after backoff; make these changes
around the references to st.lastAttempt.Add(backoff) and the host.Connect call
to ensure failed dials trigger future retries.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

pkg/p2p/client.go (2)

317-320: ⚠️ Potential issue | 🟠 Major

Still unresolved: keep only one advertise worker alive.

refreshPeerDiscovery() runs on a ticker, but advertise() already starts a long-lived re-advertisement worker. Calling it again here stacks extra advertisers for the lifetime of the client. Start one advertiser from the maintenance context and let the ticker only refresh discovery. If maintenance owns the long-lived advertiser, remove the startup peerDiscovery() call too so there is still only one loop total.

For the go-libp2p version used by this repository, does `p2p/discovery/util.Advertise` start a background re-advertisement loop until `ctx.Done()`, or is it a one-shot advertise call?

Based on learnings: Applies to **/*.go : Be mindful of goroutine leaks in Go code.

Also applies to: 399-400

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/p2p/client.go` around lines 317 - 320, startConnectionMaintenance
currently starts a maintenance context but also leads to multiple concurrent
advertisers because refreshPeerDiscovery calls advertise repeatedly while
advertise itself spawns a long-lived re-advertisement worker; to fix, ensure
only one long-lived advertise worker is created from startConnectionMaintenance
using the maintenance ctx (call advertise(ctx) once there), remove any other
startup call to peerDiscovery() so you don't spawn a separate loop, and change
refreshPeerDiscovery to only refresh discovery state (not call advertise) so it
no longer creates additional advertisers; reference functions:
startConnectionMaintenance, refreshPeerDiscovery, advertise, peerDiscovery and
the maintenanceCancel/maintenance context.

---

321-324: ⚠️ Potential issue | 🟠 Major

Still unresolved: seed the reconnect queue with already-disconnected seed peers.

This worker still only reacts to future Disconnected callbacks. A configured seed peer that never connected during startup, or dropped before Notify() was registered, never reaches reconnectCh, so the reconnect path never runs for it.

🔧 Minimal fix

 	c.maintenanceWg.Go(func() {
+		for _, sp := range c.seedPeers {
+			if c.isConnected(sp.ID) {
+				continue
+			}
+			select {
+			case c.reconnectCh <- sp.ID:
+			default:
+			}
+		}
+
 		discoveryTicker := time.NewTicker(peerDiscoveryInterval)
 		defer discoveryTicker.Stop()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/p2p/client.go` around lines 321 - 324, The worker registered in
c.maintenanceWg.Go only enqueues peers on future Disconnected callbacks, so
configured seed peers that were never connected or disconnected before Notify
was registered are never added to c.reconnectCh; fix by seeding the reconnect
queue at startup: after creating discoveryTicker in the maintenance goroutine
(inside the same block started by maintenanceWg.Go), iterate configured seed
peers (the seeds list used by the peer discovery logic), check each peer's
current connectedness via the host/peerstore/Network API (e.g.,
Host.Network().Connectedness / Peerstore.Connectedness), and send any peers that
are not currently connected into c.reconnectCh (respecting non-blocking
semantics or select with ctx.Done() to avoid blocking). Ensure this happens once
before the ticker loop so the existing reconnect worker will process
already-disconnected seed peers.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

pkg/p2p/client.go (2)

312-317: ⚠️ Potential issue | 🟠 Major

Dial workers are not tracked by maintenance waitgroup and are not time-boxed.

The goroutines spawned at Line 314 are outside maintenanceWg and use the parent maintenance context without per-dial timeout, so Close() can wait for the loop yet still race with in-flight dials. Track these workers in maintenanceWg and wrap each dial with a short context.WithTimeout.

🔧 Focused fix

 		select {
 		case c.connectSem <- struct{}{}:
-			go func(peer peer.AddrInfo) {
-				defer func() { <-c.connectSem }()
-				c.tryConnect(ctx, peer)
-			}(p)
+			peerInfo := p
+			c.maintenanceWg.Go(func() {
+				defer func() { <-c.connectSem }()
+				dialCtx, cancel := context.WithTimeout(ctx, 15*time.Second)
+				defer cancel()
+				c.tryConnect(dialCtx, peerInfo)
+			})
 		case <-ctx.Done():
 			return
 		}

Based on learnings "Applies to **/.go : Use context.Context for cancellation in Go" and "Applies to **/.go : Be mindful of goroutine leaks in Go code".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/p2p/client.go` around lines 312 - 317, The spawned dial goroutines
started when sending to connectSem are not tracked by maintenanceWg and use the
parent maintenance context without a per-dial timeout; modify the goroutine that
calls tryConnect so you increment maintenanceWg (maintenanceWg.Add(1)) before
launching, defer maintenanceWg.Done() inside the goroutine, and wrap the parent
maintenance context with a short timeout via context.WithTimeout (e.g., ctxDial,
cancel := context.WithTimeout(ctx, <reasonable-duration>); defer cancel()) and
pass ctxDial into tryConnect; keep the existing defer that releases connectSem
so the semaphore is still cleared even on timeout or cancellation.

---

300-301: ⚠️ Potential issue | 🟠 Major

c.advertise(ctx) in the ticker path can stack long-lived advertise loops.

Calling c.advertise(ctx) on every refresh tick (Line 300) is still risky if discutil.Advertise is loop-based for the lifetime of ctx; this can accumulate goroutines over time. Move advertise startup to a single call when maintenance starts, and keep the ticker for peer refresh only.

For github.com/libp2p/go-libp2p v0.48.0, does p2p/discovery/util.Advertise start a background re-advertisement loop until ctx.Done(), or is it a one-shot synchronous advertise call?

As per coding guidelines "Be mindful of goroutine leaks in Go code".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/p2p/client.go` around lines 300 - 301, The call to c.advertise(ctx)
inside the refresh ticker can spawn long-lived advertise loops repeatedly if
discutil.Advertise runs until ctx.Done(); remove the per-tick call and instead
start advertising once when maintenance begins: call c.advertise(ctx) (or spawn
a single goroutine that calls discutil.Advertise with the maintenance context)
in the maintenance startup path, keep the ticker handling only peer refresh
logic, and ensure the advertise goroutine is tied to the same cancellable
context (or returns on a done channel) so it is stopped when maintenance stops
to avoid goroutine leaks; update any start/stop or runMaintenance functions to
start that single advertise instance and to cancel it on shutdown.

pkg/p2p/client.go (1)

190-191: Consider passing context parameter to satisfy linter, even if unused.

The golangci-lint contextcheck rule flags this because startConnectionMaintenance doesn't receive the context from startWithHost. While the design is correct (maintenance uses context.Background() to decouple from caller), you could either:

1. Disable contextcheck for this line with //nolint:contextcheck
2. Accept a context.Context parameter but explicitly ignore it, documenting why

Option 1: Disable linter for this line

 	c.host.Network().Notify(c.newDisconnectNotifee())
-	c.startConnectionMaintenance()
+	c.startConnectionMaintenance() //nolint:contextcheck // maintenance uses independent background context

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/p2p/client.go` around lines 190 - 191, The linter flags
startConnectionMaintenance for not accepting the caller context; update
startConnectionMaintenance to accept a context.Context parameter (e.g., func (c
*Client) startConnectionMaintenance(ctx context.Context)) and change the call in
startWithHost from c.startConnectionMaintenance() to
c.startConnectionMaintenance(ctx). Inside startConnectionMaintenance, explicitly
ignore the passed-in ctx (e.g., _ = ctx) and add a short comment explaining you
intentionally use context.Background() internally to decouple maintenance from
the caller; this satisfies contextcheck while preserving the original design.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes